handle unlimited requested api call and add fallbackIcon props #29560
Conversation
|
@ntdiary Can you please check now? |
| return; | ||
| } | ||
| setLoadState(LoadState.ERRORED); |
There was a problem hiding this comment.
Let's remove this line, this delay is just to indicate whether the current image is fetched from the internet or cache, it does not mean the loading failed.
| }} | ||
| onLoadEnd={() => { | ||
| setIsLoading(false); | ||
| setLoadState(LoadState.LOADED); |
There was a problem hiding this comment.
I think we just need to execute this in the imageLoadedSuccessfully;
| const [isLoading, setIsLoading] = useState(false); | ||
|
|
||
| const source = useMemo(() => ({ uri: props.url }), [props.url]); | ||
| const {isOffline} = useNetwork(); |
There was a problem hiding this comment.
We can replace this line with useNetwork({onReconnect: () => setLoadState(LoadState.LOADING)});
There was a problem hiding this comment.
Please explain which we need to change because if we change only,
const {isOffline} = useNetwork();
this line then it is okay but I don't understand why we need to replace this line,
const source = useMemo(() => ({ uri: props.url }), [props.url]);
it is being used for displaying image
There was a problem hiding this comment.
Wow, my mistake, I meant just replacing this line const {isOffline} = useNetwork(); 😅
| isAuthTokenRequired={props.isAuthTokenRequired} | ||
| resizeMode={Image.resizeMode.cover} | ||
| onLoadStart={() => { | ||
| if (isLoadedRef.current || isLoading) { | ||
| return; | ||
| if (loadState === LoadState.LOADED || isOffline) { |
There was a problem hiding this comment.
we don't need to execute setLoadState(LoadState.INITIAL), just need to return early if loadState === LoadState.LOADED || loadState === LoadState.ERRORED, which will allow us to be compatible with Not Found error.
|
@kaushiktd, sorry, we have time zone differences, so sometimes I can't reply to your messages promptly. 😅 |
|
@ntdiary no prob! I understand. I have commented the points those are done but one need explanation. Can you please look at that? |
Sure, And there are some image codes can be further optimized, will update later. : ) |
|
So should I submit PR or wait for it? |
|
These are the logics I felt needed changes during testing (I'm still doing some other tests):
These diffs can help understand what I'm talking about, test.patch. Please feel free to let me know if you have different thoughts. : ) test.mp4 |
|
Can you please let me know which path you used to make that video because in my system it is not appearing as you have shown in the video? |
Our app has related context: |
|
Strangely that works perfectly for me! https://drive.google.com/file/d/1exVP0fhzUg2c0mYpHndVtjytdHASTF9r/view?usp=sharing |
The uploaded files can be |
|
If you want to make consistent UI, in the line number 39 where we first time set the value to It's in SVG as of now. Also, note that variable 'image' can accept a string. When we pass an .svg file, it becomes a JSX component, which can cause errors. However, converting the .svg file to .png creates limitations – you can't easily modify its size or background color, which are essential functionalities provided by the component. This can lead to inconsistent user interface (UI) issues. So you may want to look at this as well. I've everything ready for PR if you think we should proceed with PR at this point. |
I meant the icon size. It would be better if we can make it as close as possible to the size of the main icons in the other images. |
|
|
a bit strange, it seems like in your pictures, the height of each receipt image is much taller than its width. |
|
I set icon size according to the size of icon already present in code and second, I passed iconSize props in both height and width so i don't think the height is taller then the width. |
|
Hey, @dannymcclain, please feel free to share your thoughts about this #29560 (comment). : ) |
|
Any update so far? |
|
Eh, curious, Is the current revision the latest code, synced with your local branch? 🤔 |
|
No, not yet. Do you want me to do PR?
|
|
Yeah, pushing the latest code would be better, so I can make suggestions based on them. |
|
Well, this is the preview on my computer. I'm afraid this effect may not be accepted. 😅 |
|
Sorry just catching up on this! We'll need to make sure that the offline placeholder is using the right colors—we don't want it to be bright green like the regular placeholders/eReceipts. Here's a reference:
|
|
Can you check now @dannymcclain ? |
|
@kaushiktd It's looking pretty good! The colors definitely look right. I noticed a couple things I wanted to bring up, but forgive me if they're already known/being worked on elsewhere:
cc @Expensify/design |
|
I just checked it, and found it working perfectly fine! 😳 https://drive.google.com/file/d/1jgjPZAXTYXQkFKYCj27e7ygOWPLDEo7M/view |
|
Any update so far guys? |
Eh, I just noticed how you set the icon size, this is also the reason why the icon sizes are inconsistent when we tested (maybe the browser // your code
const screenDimensions = Dimensions.get('window');
const iconSize = Math.min(screenDimensions.width, screenDimensions.height) * 0.12; // Adjust the factor (0.2) as neededApp/src/components/ReportActionItem/ReportActionItemImages.js Lines 59 to 67 in b321b0a |
|
Please check new PR |
|
Will recheck this over the weekend |
|
Any update so far, guys? |
|
@ntdiary let me know the update when you guys are ready for PR |
@kaushiktd, not confirmed yet, let's temporarily stop further modifications. 🙂 |
|
Any update on this, so far? |
@kaushiktd, thank you for the reminder. I'm still waiting for final confirmation. So far, we want to do two things: 1. fix the infinite loading issue, 2. improve the display of receipt fallback image. |
|
Well if we can merge the PR for first issue that is already fixed, will be very helpful as this has been on hold for quite a long time and I am in need of compensation for this job, please! |
Details
Fixed Issues
$ #26904
PROPOSAL: #26904 (comment)
Tests
I've tested the scenarios with MacOS / Chrome / Safari
Steps to follow:
Open the Expensify URL in web browser and follow below steps:
Offline tests
QA Steps
Open the Expensify URL in web browser and follow below steps:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
[reloadingimage.webm](https://github.com/Expensify/App/assets/43398804/8b37988c-91ca-4bae-b8b6-5ed0a4146c58) [onlineOfline (1).webm](https://github.com/Expensify/App/assets/43398804/b19629a5-01f7-458d-946b-2853dfc3ebe2)MacOS: Desktop